Optimistic signature verification for votes and order votes #14642
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vusirikala
requested review from
zekun000,
sasha8,
ibalajiarun,
JoshLind and
gregnazario
as code owners
⏱️ 10h 19m total CI duration on this PR
🚨 2 jobs on the last run were significantly faster/slower than expected
|
vusirikala
changed the base branch from
main
to
satya/ledger_info_with_mixed_signatures
vusirikala
requested review from
sitalkedia,
danielxiangzl and
igor-aptos
and removed request for
gregnazario,
JoshLind and
sasha8
vusirikala
added
the
CICD:run-e2e-tests
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| pub fn verified_voters(&self) -> impl Iterator<Item = &AccountAddress> { | ||
| self.verified_signatures.signatures().keys() | ||
| self.signatures.iter().filter_map(|(voter, signature)| { |
There was a problem hiding this comment.
nit: can shorten as
self.signatures.iter()
.filter(|(_, sig)| sig.is_verified())
.map(|(voter, _)| voter)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
danielxiangzl
approved these changes
Oct 3, 2024
zekun000
reviewed
Oct 3, 2024
consensus/src/pending_votes.rs
Outdated
| VoteStatus::NotEnoughVotes(li) => { | ||
| write!( | ||
| f, | ||
| "LI {} has {} verified votes {:?}, {} unverified votes {:?}", |
There was a problem hiding this comment.
please don't use debug format, it spams the log
types/src/ledger_info.rs
Outdated
| Ok(_) => { | ||
| self.merge_signatures(&epoch_state.verifier, false); | ||
| self.filter_invalid_signatures(verifier, false); |
There was a problem hiding this comment.
in some sense we don't really need this since the aggregation already succeeds and returns?
There was a problem hiding this comment.
Yeah, we don't. But it still makes the data structure complete, and makes it easy in test cases to check that after aggregate_and_verify function is called, we have x number of verified signatures.
There was a problem hiding this comment.
Alin was telling me that even if the aggregation succeeds, it doesn't necessarily mean all individual signatures are valid, some validators can collude to produce two invalid signatures that can be aggregated to be valid. in that sense, for the sake of sanity, I think we probably shouldn't mark them as "valid"
| OrderVoteReceptionResult::VoteAdded(1) | ||
| ); | ||
|
|
||
| vote_0.set_verified(); |
There was a problem hiding this comment.
what are we trying to do by setting it to verified?
There was a problem hiding this comment.
Just wanted to make sure that the data structure works when it receives a mix of verified and unverified signatures.
| pending_order_votes.insert_order_vote(&vote_2, &verifier, None), | ||
| OrderVoteReceptionResult::VoteAdded(2) | ||
| ); | ||
| assert_eq!(verifier.pessimistic_verify_set().len(), 1); |
There was a problem hiding this comment.
instead, here we should check if valid signatures are set to verified?
| partial_sigs.add_signature(signers[0].author(), vote_0.signature().clone()); | ||
|
|
||
| // same author voting for the same thing -> DuplicateVote | ||
| vote_0.set_verified(); |
There was a problem hiding this comment.
Just wanted to make sure that the data structure works when it receives a mix of verified and unverified signatures.
| @@ -213,6 +213,54 @@ async fn test_no_failures() { | |||
| .unwrap(); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn test_faulty_votes() { | |||
There was a problem hiding this comment.
how did you verify this works as expected? we don't have any logs showing pessimistic validators being added?
There was a problem hiding this comment.
I have added some println! states in validator_verifier and checked that the authors that are added to pessimistic verify set are being verified pessimistically from next time.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zekun000
approved these changes
Oct 7, 2024
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
ibalajiarun
reviewed
Oct 8, 2024
| @@ -78,6 +78,7 @@ pub struct ConsensusConfig { | |||
| // must match one of the CHAIN_HEALTH_WINDOW_SIZES values. | |||
| pub window_for_chain_health: usize, | |||
| pub chain_health_backoff: Vec<ChainHealthBackoffValues>, | |||
| // Deprecated | |||
| pub qc_aggregator_type: QcAggregatorType, | |||
There was a problem hiding this comment.
Since it's local config, why not remove it?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR implements optimistic signature verification to reduce the time required to verify proposal votes and order votes.
When the optimistic signature verification feature flag is enabled, we will not verify these messages up front. We will accumulate the unverified messages, and when the accumulated voting power is higher than a threshold, we will aggregate all the signatures and verify the aggregated signature.
If the verification fails, we need to verify each individual signature. The ValidatorVerifier stores the list of authors that submitted bad messages, and will disable the optimistic signature verification for these malicious voters.
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist